Skip to content

Conversation

@mb706
Copy link
Contributor

@mb706 mb706 commented Mar 5, 2016

Also general focus search bugfixes.

levels(newdesign[[dfindex]]) = mapping[levels(newdesign[[dfindex]])]
}
}
y = infill.crit(newdesign, models, control, par.set, design, iter, ...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially slow and doesn't make use of our helpers. have a look at

par.set.disc.logic.v = filterParams(par.set, type = c("discretevector", "logicalvector"))
dfindex = getParamIds(par.set.disc.logic.v, with.nr = TRUE, repeated = TRUE)

Parameters that have discrete vector params in their requirement can
not be handled by infillOptFocus, so trying to do that now gives an
error.
@mb706
Copy link
Contributor Author

mb706 commented Mar 7, 2016

Incorporated your suggestions.

I also found out that this cannot work when there is a parameter that has a discrete vector parameter in its requirement. I think for this to work one needs to either rewrite the requirements or change something in ParamHelpers.

@jakob-r
Copy link
Member

jakob-r commented Mar 8, 2016

As @berndbischl is the inventor of foucssearch he should have a final look on it. If I see somtehing in the code I will commit.

global.y = Inf

allRequirements = extractSubList(par.set$pars, "requires", simplify = FALSE)
allUsedVars = unique(do.call(base::c, sapply(allRequirements, all.vars)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is all.vars in this context? I feel stupid, but I cannot find it.
I also find it strange to use do.call(base::c, ...). Won't unlist do the trick?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

base::all.vars returning a vector of all variables used in the expression? Unfortunately it is not perfect (ideally I would want a list of all free variables) but it is the best R is giving me as it seems.

@mb706
Copy link
Contributor Author

mb706 commented Mar 8, 2016

Incorporated your suggestions (hadn't thought of unlist(..., recursive = FALSE), thanks) and tried to be more clear in the comment.

@mb706
Copy link
Contributor Author

mb706 commented Mar 8, 2016

I don't think it is the fault of my commit diff that the check fails.

@jakob-r
Copy link
Member

jakob-r commented May 17, 2016

I think we should pull this soon, or? @berndbischl

@berndbischl
Copy link
Member

can somebody pls fix the naming issues? the styleguide is really followed.

for this, please put this in a branch, we cannot do this in a fork.
this seems to be an important bug fix.

i will proofread later then, please ping me.

and check 1000 million times if all reported problems from the issue have been converted to unit tests

@jakob-r
Copy link
Member

jakob-r commented Sep 19, 2016

Continue in #257

@jakob-r jakob-r closed this Sep 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants